-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dual Loop PID #5972
Dual Loop PID #5972
Conversation
First of all, you have to consider support for other types temperature sensors, like MAX31865. Second thing, code looks "dirty", see "sensor_type_arg_name='sensor_type', sensor_pin_arg_name='sensor_pin'", it looks more like hacks rather than something suitable for master branch. You should consider to rewrite it from scratch in more generic way. |
Any tips how I can instantiate two equals sensors in same section? As they look for the same arguments in the section |
Maybe as I'm not creating a new sensor, just putting more than one together, I should create a cluster of PrinterSensorGeneric and use it to get the temperature from each sensor set in the PrinterSensorGeneric class? |
I am not Kevin, so I can't provide the "right" solution for your case, but from my point of view it would be more interesting not to change heater section. For example we already have interface to lookup heater in heaters.py
We can provide the same interface for sensors
And for example just get it by it's name in our init method
in this case we will have to modify "setup_callback" method to work with more than one callback.
|
Thank you, your example helped me. from abc import ABC, abstractmethod
class Sensor(ABC):
def __init__(self, config):
self.printer = config.get_printer()
self._temp = self.min_temp = self.max_temp = 0.0
self._callbacks = []
...
def get_temp(self):
return self._temp
@abstractmethod
def setup_minmax(self, min_temp, max_temp):
pass
def setup_callback(self, cb):
self._callbacks.append(cb) Another idea based on yours, is to load the entire temperature_sensor intead just the sensor: def setup_heater(self, config, gcode_id=None):
heater_name = config.get_name().split()[-1]
if heater_name in self.heaters:
raise config.error("Heater %s already registered" % (heater_name,))
# Setup 1st sensor
sensor = self.setup_sensor(config)
# Setup 2nd sensor
secondary_sensor_name = config.get('secondary_sensor_name', None)
if secondary_sensor_name is not None:
full_name = "temperature_sensor " + secondary_sensor_name
secondary_sensor = self.printer.lookup_object(full_name)
else:
secondary_sensor = None
# Create heater
self.heaters[heater_name] = heater = Heater(config, sensor, secondary_sensor)
self.register_sensor(config, heater, gcode_id)
self.available_heaters.append(config.get_name())
return heater Also in the Heater class we would have a new sensor, because for now the main idea is to use with the heater bed, but it can be useful for a heated chamber for example. |
This is very useful in principle. I was thinking about a different solution, with a thermistor touching the bed and mounted inside a cork (which insulates from air perfectly), used with a macro to determine the time for the print to actually start, but a dual PID with a thermistor on the bed itself is of course better if nothing is on top of the aluminium (I have glass).
|
Thanks. Certainly seems interesting, but as high-level feedback, I'm not sure this is a good candidate for the master Klipper branch. This change would add notable config complexity and it isn't clear to me that there would be a sizeable number of people that would perform the research and reconfiguration necessary to utilize it. To be clear, it does seem interesting. You might want to consider opening a new topic on Klipper Discourse to gather feedback from other users, and to gather their testing feedback. -Kevin |
FWIW, my printer and also the higher end designs (Voron, Railcore etc) typically use silicon heating mats attached to a MIC aluminum base.
If my understanding of this PR is correct, it would considerably improve heating up times, since
I think it is a very valuable approach and would be happy to see such an option in Klipper. Edit:
|
@KevinOConnor as that would benefit many large printer with thick aluminum plates like the Voron V2 I am sure the community would find ways to get the research for the average user to a minimum. BTW RRF has that features for years. Setting it up is quit heavy but user are able to manage it
Do not ask me what all that means I ask a RRF user in the German Voron community to provide it to me. https://docs.duet3d.com/en/User_manual/Reference/Gcodes#m143-maximum-heater-temperature I personally also experimented with using the external mounted thermistor as the control thermistor for the heater and the thermistor at the silicon mate only as a safe gate to get the mate not heating too much. But was not able to get that to work reliable without modifying Klipper. so the PR would be really appreciated. |
Thanks for the feedback. FWIW, I also have a Voron2 with two thermistors monitoring bed temperature ( https://github.com/KevinOConnor/voron2-mods/blob/master/bed/bed.md ). So I understand what is being reported. In case anyone is curious, I currently use the thermistor on the bed heater for the PID, my PRINT_START macro sets a high bed target temperature during initial heating, it uses TEMPERATURE_WAIT on the bed plate thermistor, and then lower the target temperature. This provides a heating curve similar to those shown at the top of this PR. As others have observed, during printing there is a reliable static offset between the heater thermistor and the bed plate thermistor. My observation is that there are several different ways to handle this type of bed setup. Both with code changes and without code changes. I think more feedback (and more user test results) would be needed before it would make sense to merge a change into Klipper. I don't think enough people will see this PR to get to that point. I think Klipper Discourse is a better place for that discussion and for gathering feedback. -Kevin |
FWIW, it's easy to tweak the TEMPERATURE_WAIT on the bed plate thermistor to avoid overshoot. In case anyone is curious, here's my macro:
-Kevin |
You are right. I mislead myself graphically. In fact my overshoot was only 4°C. This is what I have been using to play around:
Edit: The safeguard against too high |
Yes, it is noticeable, in my normal conditions my heater can heat up to 150ºc, so for example to reach 100ºc in the surface I can take the advantage from this extra heat.
Unfortunately not, my pid was tuned for 100ºc, so for lower temperature like 60ºc I have a overshoot around 1.5ºC, for a 100ºC target, my overshoot is around 0.1ºC |
The main point of this PR is to get as close as possible the desired temperature need for printing some material, there is no need to know the offsets between the heater and surface, and that offset sometimes can change because some external factor, like the fans below the bed for heat soak. After the tips from @The-Futur1st , the implementation got simpler, but I didn't update the documentation after the refactoring. I didn't know about Klipper Discourse, I will start a discussing there to get the validation from more users. Thanks you guys |
For info: I use the PWM value to define when the bed has actually reached a stable temperature: even after the (only) thermistor reaches it, the PWM takes longer to decrease until it is constant, because there is still heat flowing up. Of course given the nice macros, I'll change my approach, but I thought it was good to mention it. |
Added dual_loop_pid into documentation Signed-off-by: Rodrigo Andrade <rodrigormda@hotmail.com>
Added an optional sensor into heater class, this sensor can be added from printer.lookup_object method Signed-off-by: Rodrigo Andrade <rodrigormda@hotmail.com>
ControlDualLoopPID uses two ControlPID class Signed-off-by: Rodrigo Andrade <rodrigormda@hotmail.com>
ControlAutoTune also tunes ControlDualLoopPID Signed-off-by: Rodrigo Andrade <rodrigormda@hotmail.com>
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
Unfortunately a reviewer has not assigned themselves to this GitHub Pull Request and it is therefore being closed. It is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
@rodrigo2019 did you at least do the self-review? did you ask in the Dicourse forum and in Discord whether someone could perform the review for you? it's a pity this feature gets lost. |
@dewi-ny-je yes, I did a self-review, before the github actions message you can see my force-push updating the documentation e parts of the code. And I didn't ask to anybody to review this PR, because of this part in the guideline: |
Thanks for working on this and sharing your results. I do think it is interesting. However, I don't think there is consensus on this feature at this time. So, I think it is best to track the conversation on Discourse instead of github. -Kevin |
The idea behind the following PR is to have a more accurate bed surface temperature
Usually users who have big and thick beds have an offset between the bed surface and the thermistor temperature located on the heater, also they usually need to wait some time for the bed surface to reach the thermal equilibrium with the heater.
This PR allow the user set a new control algo that uses two sensors, one for the bed surface and one for the heater.
This new algorithm uses two PID loops, one for controlling the heater and one for controlling the bed surface, it works using the minimum control values beetwen the both PID loops.
The main target for the heater will be always the bed surface, the maximum heater temperature is set on the heater section.
Here we can see a more detailed simulation:
The temperature on the heater (orange curve) was set to not exceed 100ºC while the bed surface target was on 60ºC
And here we can see a practical situation on my printer, which have a bed size 350mmx350mmx8mm. In this situation the same parameters was set as in the simulation
Here is how I setup the sensors in my bed:
and here how my configuration looks like now: